Stop the #854 shim warning on causalml's own internal fit calls - #988
Merged
Conversation
The deprecation shim suppresses re-entrant warnings with a flag stored on the instance, so it covers self.fit -> self.predict nesting but not the forest -> tree hop: the flag is set on the forest while the warning fires on each tree it fits. CausalRandomForestRegressor.fit(X=X, y=y, treatment=treatment) — an all-keyword call, written exactly the way the warning asks for — therefore emitted one FutureWarning per tree, 100 of them at the default n_estimators, telling the caller to do what they had already done. Nothing the caller could change would silence it. UpliftRandomForestClassifier had the same behavior (default 10). Make the forests pass y and treatment to their trees by keyword. This is the same migration #982 applies to tests/, applied to the library's own call sites; the epic has no ticket covering those. An empirical probe over eleven public entry points (both forests, both trees, the S/T/X/R/DR learners, estimate_ate and fit_predict), all called with keywords, goes from two offending sites to zero. The two regression tests fail without the change. The same-instance super().fit(X, treatment, y, ...) calls in upliftforest.py, uplifttree.py and rlearner.py are left alone: the guard already covers them and they warn nobody today. They do need to change when #985 flips the signature order, or treatment will silently land in y. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeongyoonlee
marked this pull request as ready for review
July 31, 2026 11:06
jeongyoonlee
added a commit
that referenced
this pull request
Aug 2, 2026
#982 asks for a test run with zero arg-order FutureWarnings. After migrating `tests/` it still emits 10, and none of them come from a test: they come from `causalml/metrics/sensitivity.py` calling its own shimmed helpers positionally. This is the same instance-scoped re-entrancy gap #988 fixed for the forests, one module over. `Sensitivity.sensitivity_analysis` takes neither `treatment` nor `y` -- it reads them from a DataFrame by column name -- so the shim leaves it unwrapped and no `_in_arg_order_call` guard is ever set. The `get_prediction` / `get_ate_ci` / `get_potential_outcome_predictions` calls it makes internally therefore warn on their own account: >>> Sensitivity(df=df, ..., learner=BaseXLearner(...)).sensitivity_analysis( ... methods=["Random Cause", "Random Replace"], sample_size=0.5) FutureWarning: Passing `treatment` and/or `y` to get_prediction() by position ... FutureWarning: Passing `treatment` and/or `y` to get_ate_ci() by position ... There is no positional argument in that call for the caller to fix, and no way to silence it. Ten internal call sites in `sensitivity.py` now pass `treatment`/`y` by keyword. Behaviour is unchanged -- the positional order is still `(X, p, treatment, y)` and this does not pre-judge #980's open question 2 about whether the `Sensitivity` helpers are in scope for the v1.0 flip. Keyword calls are correct either way. Verified: - An AST check over `sensitivity.py` reports zero remaining internal calls that pass `treatment`/`y` positionally. - The reproduction above goes from 2 warnings to 0. - `tests/test_sensitivity.py`: 20 passed. `tests/test_fit_arg_order.py`: 55. - The new regression test fails with the library change reverted and passes with it, so it pins the fix rather than passing vacuously. - `black --check` clean. Refs #854, #988. Part of #980; needed for #982's acceptance criteria. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
10 tasks
jeongyoonlee
added a commit
that referenced
this pull request
Aug 2, 2026
#982 asks for a test run with zero arg-order FutureWarnings. After migrating `tests/` it still emits 10, and none of them come from a test: they come from `causalml/metrics/sensitivity.py` calling its own shimmed helpers positionally. This is the same instance-scoped re-entrancy gap #988 fixed for the forests, one module over. `Sensitivity.sensitivity_analysis` takes neither `treatment` nor `y` -- it reads them from a DataFrame by column name -- so the shim leaves it unwrapped and no `_in_arg_order_call` guard is ever set. The `get_prediction` / `get_ate_ci` / `get_potential_outcome_predictions` calls it makes internally therefore warn on their own account: >>> Sensitivity(df=df, ..., learner=BaseXLearner(...)).sensitivity_analysis( ... methods=["Random Cause", "Random Replace"], sample_size=0.5) FutureWarning: Passing `treatment` and/or `y` to get_prediction() by position ... FutureWarning: Passing `treatment` and/or `y` to get_ate_ci() by position ... There is no positional argument in that call for the caller to fix, and no way to silence it. Ten internal call sites now pass every argument by keyword. Behaviour is unchanged -- the positional order is still `(X, p, treatment, y)` and this does not pre-judge #980's open question 2 about whether the `Sensitivity` helpers are in scope for the v1.0 flip. Keyword calls are correct either way. Verified: - An AST check over `sensitivity.py` reports zero remaining internal calls that pass anything positionally to a shimmed helper. - The reproduction above goes from 2 warnings to 0. - The new regression test fails with the library change reverted and passes with it, so it pins the fix rather than passing vacuously. - `black --check` clean. Refs #854, #988. Part of #980; needed for #982's acceptance criteria. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Follow-up to #975. The deprecation shim suppresses re-entrant warnings with a flag stored on the instance, so it covers
self.fit→self.predictnesting but not the forest → tree hop: the flag is set on the forest while the warning fires on each tree it fits.The result is a warning the caller cannot act on or silence:
That call is all-keyword — written exactly the way the warning asks for — and it emits one FutureWarning per tree: 100 at the default
n_estimators.UpliftRandomForestClassifierbehaves the same way (default 10).This makes the forests pass
yandtreatmentto their trees by keyword. It is the same migration #982 applies totests/, applied to the library's own call sites — the epic (#980) has no ticket covering those.Call sites changed
causalml/inference/tree/causal/causalforest.py:136causalml/inference/tree/causal/causalforest.py:145causalml/inference/tree/_uplift/upliftforest.py:73Verification
An empirical probe over eleven public entry points (both forests, both trees, the S/T/X/R/DR learners,
estimate_ate,fit_predict), all called with keywords, attributes each warning to its originating library line viastacklevel=2: 2 offending sites before, 0 after.tests/test_fit_arg_order.py: 54 passed.tests/test_causal_trees.py tests/test_uplift_trees.py tests/test_uplift_trees_kernel.py tests/test_serialization_extended.py: 135 passed.black --checkclean.Left alone, deliberately
The same-instance
super().fit(X, treatment, y, ...)calls inupliftforest.py:342,uplifttree.py:567andrlearner.py:913. The guard already covers them and they warn nobody today. They do need to change under #985 — once the signature order flips,treatmentsilently lands iny.Note on why this was not caught
The repo sets no
filterwarningsanywhere, soFutureWarningis never an error and the affected tests pass on a normal run. A deprecation shim without a warnings-as-errors lane is untested by construction; worth pairing with the CI work in #977.Types of changes
Checklist
🤖 Generated with Claude Code